Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds AHP (Analytic Hierarchy Process) functionality to the application, implementing a multi-criteria decision analysis tool that can compute weighted rankings of alternatives based on criteria comparisons.
Changes:
- Added mathjs library dependency for matrix operations
- Implemented AHP computation utilities including matrix validation, priority vector calculation, and consistency ratio computation
- Created API endpoint
/api/v1/ahpfor data-driven AHP calculations
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/validateMatrix.js | Matrix validation for reciprocal matrices used in AHP |
| src/utils/pairwiseMatrix.js | Builds pairwise comparison matrices from raw data |
| src/utils/computePriorityVector.js | Computes priority vectors using geometric mean method |
| src/utils/computeConsistancyRatio.js | Calculates consistency ratio for pairwise matrices |
| src/utils/ahs.js | Core AHP computation logic |
| src/controllers/ahpController.js | HTTP controller for AHP endpoint |
| src/routes/ahpRoutes.js | Express router for AHP routes |
| server.js | Registers AHP routes in the application |
| package.json | Adds mathjs dependency |
| package-lock.json | Lock file updates for mathjs and its dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { create, all } from "mathjs"; | ||
|
|
||
| const math = create(all, {}); |
There was a problem hiding this comment.
The mathjs instance is created but never used in this file. The math.multiply function is called on line 14, but since mathjs is imported and configured, you should verify if this configuration is necessary. If the multiply function works without the custom math instance, consider removing the unused constant.
| import { create, all } from "mathjs"; | ||
| import matrixValidation from "./validateMatrix.js"; | ||
| import weightVectorGeometricMean from "./computePriorityVector.js"; | ||
| import { consistencyRatio } from "./computeConsistancyRatio.js"; | ||
|
|
||
| const math = create(all, {}); | ||
|
|
There was a problem hiding this comment.
The mathjs instance is created but never used in this file. Since no mathjs operations are performed directly in this module, consider removing this unused import and constant.
| import { create, all } from "mathjs"; | |
| import matrixValidation from "./validateMatrix.js"; | |
| import weightVectorGeometricMean from "./computePriorityVector.js"; | |
| import { consistencyRatio } from "./computeConsistancyRatio.js"; | |
| const math = create(all, {}); | |
| import matrixValidation from "./validateMatrix.js"; | |
| import weightVectorGeometricMean from "./computePriorityVector.js"; | |
| import { consistencyRatio } from "./computeConsistancyRatio.js"; |
| console.log("Validating matrix:", matrix); | ||
|
|
There was a problem hiding this comment.
Debug console.log statement should be removed from production code. This could log potentially large matrices and cause performance issues. If logging is needed, consider using a proper logging library with configurable log levels.
| console.log("Validating matrix:", matrix); | |
| // debug: log matrix shapes (removed after diagnosing issues) | ||
| try { | ||
| // avoid large dumps; log dimensions and a sample | ||
| const altKeys = Object.keys(alternativeMatrices); | ||
| const altShapes = altKeys.map(k => ({ key: k, size: alternativeMatrices[k]?.length ?? 0 })); | ||
| console.log('computeDataDrivenAHP: criteriaMatrix size=', criteriaMatrix.length, 'alternativeMatrices=', altShapes); | ||
| } catch (e) { | ||
| console.error('computeDataDrivenAHP debug error', e); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Debug console.log statement should be removed from production code. The comment on line 51 mentions 'removed after diagnosing issues' but the log is still present. Consider using a proper logging library with configurable log levels or remove this debug code.
| // debug: log matrix shapes (removed after diagnosing issues) | |
| try { | |
| // avoid large dumps; log dimensions and a sample | |
| const altKeys = Object.keys(alternativeMatrices); | |
| const altShapes = altKeys.map(k => ({ key: k, size: alternativeMatrices[k]?.length ?? 0 })); | |
| console.log('computeDataDrivenAHP: criteriaMatrix size=', criteriaMatrix.length, 'alternativeMatrices=', altShapes); | |
| } catch (e) { | |
| console.error('computeDataDrivenAHP debug error', e); | |
| } | |
| } | ||
|
|
||
| try { | ||
| console.log("Start running AHP computation"); |
There was a problem hiding this comment.
Debug console.log statement should be removed from production code. Consider using a proper logging library consistent with the rest of the application, or remove this debug statement.
| console.log("Start running AHP computation"); |
| const { criteriaNames, alternativeNames, dataMatrix, benefitFlags } = req.body || {}; | ||
|
|
||
| if (!req.body) { | ||
| return res.status(400).json({ error: 'Request body is required. Please send JSON with dataMatrix, criteriaNames, alternativeNames, and benefitFlags.' }); | ||
| } | ||
|
|
There was a problem hiding this comment.
The check on line 6 is redundant since req.body is already accessed on line 4 with a fallback to an empty object. If req.body is null/undefined, the destructuring on line 4 will use {} and the variables will be undefined. The check should be moved before the destructuring, or specific parameter validation should be added instead.
| const { criteriaNames, alternativeNames, dataMatrix, benefitFlags } = req.body || {}; | |
| if (!req.body) { | |
| return res.status(400).json({ error: 'Request body is required. Please send JSON with dataMatrix, criteriaNames, alternativeNames, and benefitFlags.' }); | |
| } | |
| const body = req.body; | |
| if (!body || typeof body !== 'object') { | |
| return res.status(400).json({ error: 'Request body is required. Please send JSON with dataMatrix, criteriaNames, alternativeNames, and benefitFlags.' }); | |
| } | |
| const { criteriaNames, alternativeNames, dataMatrix, benefitFlags } = body; |
| if (!Array.isArray(dataMatrix)) { | ||
| return res.status(400).json({ error: 'dataMatrix must be an array.' }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Incomplete input validation. The controller only validates that dataMatrix is an array but doesn't validate other required parameters (criteriaNames, alternativeNames). These are required by computeDataDrivenAHP and will cause unclear error messages if missing. Add validation for all required parameters.
| if (!Array.isArray(criteriaNames) || criteriaNames.length === 0) { | |
| return res.status(400).json({ error: 'criteriaNames must be a non-empty array.' }); | |
| } | |
| if (!Array.isArray(alternativeNames) || alternativeNames.length === 0) { | |
| return res.status(400).json({ error: 'alternativeNames must be a non-empty array.' }); | |
| } |
| @@ -0,0 +1,30 @@ | |||
| import { create, all } from "mathjs"; | |||
There was a problem hiding this comment.
Filename has a typo: 'Consistancy' should be 'Consistency'. The file is named 'computeConsistancyRatio.js' but should be 'computeConsistencyRatio.js'.
| @@ -0,0 +1,28 @@ | |||
| import { computeDataDrivenAHP } from "../utils/pairwiseMatrix.js"; | |||
|
|
|||
| export const dataDrivenAHP = (req, res) => { | |||
There was a problem hiding this comment.
The new AHP controller lacks test coverage. All other controllers in the codebase (branchController, messageController, thirdPartiesController, etc.) have corresponding test files in src/tests/. Consider adding comprehensive tests for the AHP controller covering successful computation, validation errors, and error handling.
| import express from 'express'; | ||
| import { dataDrivenAHP } from '../controllers/ahpController.js'; | ||
|
|
||
| const router = express.Router(); |
There was a problem hiding this comment.
Import style is inconsistent with other route files in the codebase. Other route files use destructured imports like import { Router } from 'express' (see branch.js line 2). Consider using the same pattern for consistency.
| import express from 'express'; | |
| import { dataDrivenAHP } from '../controllers/ahpController.js'; | |
| const router = express.Router(); | |
| import { Router } from 'express'; | |
| import { dataDrivenAHP } from '../controllers/ahpController.js'; | |
| const router = Router(); |
vindyakodithuwakku02
left a comment
There was a problem hiding this comment.
CI fixed and checks passed.
No description provided.